chore(pd): sync get_lr from pt to pd #5144
Conversation
📝 WalkthroughWalkthroughThe changes refactor the learning rate module in the training pipeline by replacing an import from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/pd/train/training.py (1)
241-245: Avoid mutatingconfigvia in-place updates tolr_params.
Make a shallow copy before injectingstop_stepsto prevent side effects if the dict is reused/logged elsewhere.Proposed diff
def get_lr(lr_params: dict[str, Any]) -> BaseLR: - lr_params["stop_steps"] = self.num_steps - self.warmup_steps - lr_schedule = BaseLR(**lr_params) - return lr_schedule + lr_params = dict(lr_params) # avoid mutating config in-place + lr_params["stop_steps"] = self.num_steps - self.warmup_steps + return BaseLR(**lr_params)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/pd/train/training.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4219
File: deepmd/utils/learning_rate.py:48-53
Timestamp: 2024-10-15T22:22:24.889Z
Learning: Methods in `deepmd/utils/learning_rate.py` that return NumPy scalar types should have return type annotations using the corresponding NumPy types, such as `np.float64`.
🧬 Code graph analysis (1)
deepmd/pd/train/training.py (2)
deepmd/dpmodel/utils/learning_rate.py (1)
BaseLR(21-49)deepmd/pt/train/training.py (1)
get_lr(269-272)
🔇 Additional comments (1)
deepmd/pd/train/training.py (1)
33-35: Compatibility check:BaseLR(**lr_params)may require a type/variant key that older PD configs didn’t provide.
If PD configs previously relied on an implicitLearningRateExp, please confirm the PD config schema/examples always include the discriminatorBaseLR.__new__uses to pick a concrete schedule (otherwise this becomes a runtime failure).
There was a problem hiding this comment.
Pull request overview
This PR synchronizes the get_lr function implementation in the PaddlePaddle (pd) backend with the PyTorch (pt) implementation, enabling support for multiple learning rate schedulers beyond just the exponential decay type.
Changes:
- Updated
get_lrfunction to use the plugin-basedBaseLRclass instead of directly instantiatingLearningRateExp - Added type hints to the function signature
- Removed the assertion limiting learning rate types to only "exp"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5144 +/- ##
==========================================
- Coverage 81.93% 81.93% -0.01%
==========================================
Files 712 712
Lines 72895 72894 -1
Branches 3616 3616
==========================================
- Hits 59729 59727 -2
- Misses 12001 12004 +3
+ Partials 1165 1163 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.